VDBObject : Don't bake in automatic metadata when querying metadata #1423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Added a fix commit so that worldBound returns correct results in the scenario that the metadata isn't set. This kind of confirms though that finding the right approach for all this stuff may not be trivial. evalActiveVoxelBoundingBox seems potentially expensive to possibly end up calling repeatedly on the same VDB every time you need the bbox ... and it only works if the grids are fully loaded? I do start to wonder whether there could be merit in the opposite approach ( make sure that everything that accesses the metadata goes through a wrapper that is thread safe and adds in the standard stats if they aren't present ). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses proposal 2) from John's comment here:
GafferHQ/gaffer#5923 (comment)
I think a PR is probably the most direct way to discuss it, though I'm not entirely convinced that just merging this is necessarily correct.
I think the argument is completely sound that the previous behaviour was wrong - we were calling this function assuming it was const, and this was resulting in things getting written in the cache in a completely non-threadsafe way. This definitely seems more correct.
But just doing this results in a worse experience for users. Seeing the voxel counts and bounding box sizes in the Gaffer Scene Inspector seems quite useful.
Perhaps the Gaffer inspector should compute the bounding box and voxel counts explicitly, in addition to showing the metadata? That is pretty independent from this, but maybe we should have a plan for it before we merge this.